test: add integration tests for "pebble run" #497
Conversation
There was a problem hiding this comment.
The approach overall makes sense, though I wonder: do build flags allow you to fudge the code under test somehow?
I can see how this allows running integration tests overall, but I'm a bit unclear about how this helps testing code that needs root or what not.
P.S. maybe get someone from Juju to review the go bits?
|
Resolving all the above comments, and here is a list of test cases for Pebble Run Tests (
|
|
I have restructured the files and added the tests mentioned above. Note that I didn't strictly follow the "rule of 3" when creating those helper functions, some are only used twice, some even just once. The reason is that if some code weren't put in a separate function, the test functions would become much longer and harder to read. So, I kept them as they were. Instead of thinking of them as helper functions, think of them as a way to refactor the tests to improve readability, and if they don't fit future needs, we can refactor them later. Todo:
|
There was a problem hiding this comment.
Thanks for this -- I think it's heading in the right direction. Lots of comments, but they're basically about how we structure the helpers and make the tests more obvious. I think in general it's better to have fewer, more generic helpers. One tell-tale sign is that same of the names start to get long or specific, like isPortUsedByProcess (very specific to a particular test), writeIdentitiesFile (doesn't actually do anything specific to identities), or runPebbleCmdAndCheckOutput (often an "and" in a function name means you should split it). Happy to discuss any of these further on video if you want.
I realise this is somewhat subjective, but I think mere length is okay. I definitely disagree about "harder to read" -- I think several of the helpers obscure the logic of the test and make it unclear what it's actually testing. I think there are some minor tweaks we can make to the structure and names to help with this, and I've argued my case in the comments above. |
benhoyt
changed the title
There was a problem hiding this comment.
FWIW, I just ran the integration tests with the pebble daemon running in another window, and got this error:
$ go test -count=1 -tags=integration ./tests/
--- FAIL: TestHttpPort (3.05s)
run_test.go:113: Error waiting for logs: timed out waiting for log: Started daemon
It seems to me the two instances should be completely independent, as the integration tests point to a temporary PEBBLE directory. Any ideas why this would fail?
I saw this comment at last after fixing all comments above, and after the fixes, I could not reproduce. Could you confirm? |
|
Regarding the |
There was a problem hiding this comment.
Looking good, thanks!
One remaining thing I noticed after running the integration tests -- they don't seem to clean up after themselves very well. After running them, I get:
$ ps aux | grep sleep
ben 137333 0.0 0.0 2800 1536 pts/2 S 08:56 0:00 /bin/sh -c touch /tmp/TestStartupEnabledServices3374203583/001/svc1; sleep 1000
ben 137334 0.0 0.0 2800 1536 pts/2 S 08:56 0:00 /bin/sh -c touch /tmp/TestStartupEnabledServices3374203583/001/svc2; sleep 1000
ben 137337 0.0 0.0 8288 1920 pts/2 S 08:56 0:00 sleep 1000
ben 137338 0.0 0.0 8288 1920 pts/2 S 08:56 0:00 sleep 1000
ben 137373 0.0 0.0 2800 1536 pts/2 S 08:56 0:00 /bin/sh -c echo 'hello world'; sleep 1000
ben 137374 0.0 0.0 8288 1920 pts/2 S 08:56 0:00 sleep 1000
ben 137384 0.0 0.0 2800 1536 pts/2 S 08:56 0:00 /bin/sh -c echo 'hello world'; sleep 1000
ben 137385 0.0 0.0 8288 1920 pts/2 S 08:56 0:00 sleep 1000
Any idea why? Maybe we can do some logging to see why. I would have thought sending SIGINT should cause Pebble to stop the running services before exiting, but it's clearly not doing that (properly, at any rate).
One thing we could do to mitigate it (but not fix it, so we should still look into it) is changing the sleep 1000 to sleep 10 -- that's still plenty long enough for these tests, but at least they'll exit sooner themselves.
I'll also ask Harry to review this, as he originally created the issue.
|
Agreed with all of Harry's comments (but a slightly different explicit approach than |
|
Reply to the above comments:
|
Can you please spend a bit of time looking into why is? I just don't want to paper over something if it's an actual bug. |
|
I looked into this further, and debugged by printing out the Pebble/stderr logs (and running the tests with In the meantime, it's probably best if we wait till the "startup: enabled" services are started when the daemon starts up, and the simplest way to do this now is to wait for the "Started default services ..." log. Note that the full log looks like this. I recommend the following diff, which cleans up nicely in my tests: diff --git a/tests/run_test.go b/tests/run_test.go
index c73c365..c98c693 100644
--- a/tests/run_test.go
+++ b/tests/run_test.go
@@ -49,7 +49,8 @@ services:
createLayer(t, pebbleDir, "001-simple-layer.yaml", layerYAML)
- _, _ = pebbleRun(t, pebbleDir)
+ _, stderrCh := pebbleRun(t, pebbleDir)
+ waitForLog(t, stderrCh, "pebble", "Started default services", 3*time.Second)
waitForFile(t, filepath.Join(pebbleDir, "svc1"), 3*time.Second)
waitForFile(t, filepath.Join(pebbleDir, "svc2"), 3*time.Second)
@@ -141,6 +142,7 @@ services:
stdoutCh, stderrCh := pebbleRun(t, pebbleDir, "--verbose")
waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
waitForLog(t, stdoutCh, "svc1", "hello world", 3*time.Second)
+ waitForLog(t, stderrCh, "pebble", "Started default services", 3*time.Second)
}
// TestArgs tests that Pebble provides additional arguments to a service
@@ -166,6 +168,7 @@ services:
)
waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
waitForLog(t, stdoutCh, "svc1", "hello world", 3*time.Second)
+ waitForLog(t, stderrCh, "pebble", "Started default services", 3*time.Second)
}
// TestIdentities tests that Pebble seeds identities from a file |
|
I've opened #502 to track having Pebble terminate services in "starting" state as well. |
|
While testing the leaked "sleep" issue, I found another place that leaks a sleep. Since it's not related to this feature, I will merge this PR and continue debugging it in another branch. |
A PoC to solve the Pebble integration test, see issue here.
Two issues I want to call out:
testing: warning: no tests to run.Maybe this is possible but I haven't made it work yet, and I think this could be the reason: as per the Golang Doc, the build tag lists the conditions under which a file should be included in the package, and maybe this is the reason why it doesn't work with suite.
It means we can't do something like
func (s *IntegrationSuite) TestXXX(c *C), but can only dofunc TestXXX(t *testing.T); which is OK I think, just something worth mentioning.The daemon is a long-running process and we can't wait for it to "finish". Here I used something like "if there are no new logs in the past second, kill the process and return". It doesn't feel ideal to me.
I had another draft where I simply passed a value for timeout into it, sleep, then kill.
Both worked, but I'm not sure if this is the best we can do here.